Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat: developed contribution section in project page #173

Open
wants to merge 13 commits into
base: develop
Choose a base branch
from

Conversation

Bucky25
Copy link
Contributor

@Bucky25 Bucky25 commented Dec 6, 2020

Description

Used GitHub API to get commit data and made a contribution heat map for last 30 days
patch #80

Fixes #80

Type of Change:

Delete irrelevant options.

  • Code

Code/Quality Assurance Only

  • Bug fix (non-breaking change which fixes an issue)

  • New feature (non-breaking change which adds functionality pre-approved by mentors)

How Has This Been Tested?

I have tested the implementation and responsiveness.
here is the deployed link.
https://bucky25.github.io/anitab-org.github.io/

Checklist:

  • My PR follows the style guidelines of this project
  • I have performed a self-review of my own code or materials
  • I have attached link of deployed site.
  • Any dependent changes have been merged

Code/Quality Assurance Only

  • My changes generate no new warnings

@nandini45
Copy link
Member

@gaurivn can u test this PR??

import { withCard } from './../../../Decorators/Card';
import Badge from './CardBadge';

const EventCard = ({ props, isOver }) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove isOver as it's not used anywhere

<Badge text={props.location} link={require('./../../../assets/events_and_highlights/location.png')} />
<Badge text={props.timings} link={require('./../../../assets/events_and_highlights/time.png')} />
<View style={{marginTop: 32}}>
{props.description.map((detail,index) => (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use index as key in the child when using map

marginTop: 30,
}}
>
{events_highlight.sections[1].events.map((event_detail,index) => (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove index as it's not being used anywhere

Copy link
Contributor

@tichnas tichnas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Bucky25 I've requested some changes which I think should be done.
Also, please try to avoid inline styles and class components for consistency in the code and put all margins/paddings as a power of 2.

Thanks :)

}
];

function Contribution () {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use arrow function here for consistency as it's being used everywhere else.

@@ -5,6 +5,9 @@ import { View } from 'react-native';
import ImageTextSection from './../ImageTextSection';
import { getProjects } from './../../content/projects_content';
import ProjectCard from './ProjectCard';
import Calander from './Contribution';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change Calander to Calendar

Copy link
Contributor

@tichnas tichnas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete package-lock.json file as it isn't required for yarn package manager (which we are using).
Also, instead of using var, use const or let.

import { View, Text, StyleSheet } from 'react-native';
import ContributionBox from './Box';

class ContributionRow extends React.Component {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change class component to functional component (preferably arrow function).

I know both are same in terms of performance, but functional components are more concise and modern (and are used everywhere else). They don't have any disadvantages but may get optimized in future. Please refer https://www.twilio.com/blog/react-choose-functional-components for more information.

Do let me know if you face any trouble with functional components.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok i will use useEffect there and it will be done

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Bucky25 Are you saying about componentDidMount? If yes, there is a React hook called useEffect which you can use (refer to the link shared above)

@isabelcosta isabelcosta added the Status: Needs Review PR needs an additional review or a maintainer's review. label Dec 22, 2020
Copy link
Contributor

@annabauza annabauza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Icons are wrong. @Vuyanzi @Sonal240 @simranaggarwal1999 can any of you provide proper icons please. Thanks a lot.

@@ -12,7 +12,7 @@ function Content({ selected, titles }) {
return <AboutUs />;
} else if (selected === 3) {
return <Projects />;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
}
}

var hue = 0;
switch (true) {

case (props < 2):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't we just use interpolation here?

import ContributionRow from './ContributionRow';
import ContributionBox from './Box';

const data = [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move this to content folder please.

}
<View style={styles.description}>
<Text style={styles.text}>Less</Text>
<ContributionBox props={1} />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it will be good to rename it from props. The name is misleading.

@annabauza
Copy link
Contributor

is this the same PR as #171 ?

@annabauza annabauza added Status: Changes Requested Changes are required to be done by the PR author. and removed Status: Needs Review PR needs an additional review or a maintainer's review. labels Dec 23, 2020
@Bucky25 Bucky25 requested a review from annabauza January 1, 2021 15:33
@Vishal-raj-1
Copy link

@Bucky25 Please squash your commits into one commit !!

@annabauza
Copy link
Contributor

@Vishal-raj-1 we squash when merge. No need to squash.

@Vishal-raj-1
Copy link

@annabauza that's great !!

@Bucky25
Copy link
Contributor Author

Bucky25 commented Feb 1, 2021

@Vishal-raj-1 we squash when merge. No need to squash.

@annabauza thanks

@nandini45
Copy link
Member

@Bucky25 any pending work needs to be done in the pr??

@Bucky25
Copy link
Contributor Author

Bucky25 commented Feb 18, 2021

@Bucky25 any pending work needs to be done in the pr??

@nandini45 i think its done i have done all the changes required ?

@nandini45
Copy link
Member

@Bucky25 yes i will ping Anna again for her review
sorry for the delay. you can look for other issues if you want
thanks

Copy link
Contributor

@annabauza annabauza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

100


const ContributionBox = ({ commitCount }) => {
var hue = 0;
switch (true) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is dodgy i think simple if else would do here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done @annabauza please check it

@Vishal-raj-1
Copy link

@Bucky25 It contains conflicting files. Resolve it !!

as files already present
@Bucky25
Copy link
Contributor Author

Bucky25 commented Feb 19, 2021

@annabauza please remove these files:
src/assets/events_and_highlights/calendar.png
src/assets/events_and_highlights/location.png
src/assets/events_and_highlights/time.png
as they are not correct png for icons then merging is all fine

@nandini45
Copy link
Member

@annabauza can you please provide your feedback ?

@keshakaneria
Copy link
Member

@Bucky25 any updates about solving the conflicts?

@Bucky25 Bucky25 requested a review from annabauza May 5, 2021 20:11
@Bucky25
Copy link
Contributor Author

Bucky25 commented May 5, 2021

@Bucky25 any updates about solving the conflicts?

i already mentioned that these files are existing in the repo and are different from pr because new one are that png files .

@nandini45
Copy link
Member

@annabauza can you review the PR please?

@nandini45
Copy link
Member

@isabelcosta Can you review the PR ?

@isabelcosta
Copy link
Member

@isabelcosta Can you review the PR ?

I can try but my frontend skills are a bit rusty, could you ask Zulip to see if anyone wants to help. @nandini45

@isabelcosta
Copy link
Member

@Bucky25 are you still interested in finishing this issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Changes Requested Changes are required to be done by the PR author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CODING:develop the contribution section in the project page.
7 participants